datastore(fix): Fixes previous PR which was inadvertently creating multiple websockets. #2719
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2719 +/- ##
==========================================
+ Coverage 42.85% 42.87% +0.01%
==========================================
Files 905 905
Lines 29098 29099 +1
Branches 4142 4142
==========================================
+ Hits 12471 12476 +5
+ Misses 15269 15267 -2
+ Partials 1358 1356 -2 |
There was a problem hiding this comment.
At first I was concerned because we would lose the ability to have multiple threads waiting for the connection concurrently, but that isn't a concerned since waitForConnection will return immediately after connected - so threads are simply waiting for webSocket lock instead, and there's no impact to throughput.
Removal of synchronize on releaseSubscriptions also looks good, and allows releasing multiple subscriptions in parallel to speed up the shutdown process.
I think my main question is whether the isDisconnected() check even makes sense while establishing subscriptions. If the socket is closed due to failure then error handling should have already been in progress, otherwise if it was a graceful close then webSocketListener will already be null. Recognize that isn't really part of this PR but something worth thinking about.
|
👍 Looks good in testing. Subscription establishment is much faster than in |
Issue #, if available:
Description of changes:
Continuation of #2718
The previous PR was inadvertently creating multiple websockets. This is because the websocket lock had a check
if (webSocketListener == null || webSocketListener.isDisconnectedState())which would create a new ws. Because we were rapidly sending, every subscription request was in a disconnected state.This PR waits for the ws connection to open, before it adds the subscriptions
Additionally, I also discovered that disconnecting the websocket takes too long due to the same issue we observed in #2718 with the creation of the subscriptions.
How did you test these changes?
(Please add a line here how the changes were tested)
Documentation update required?
General Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.